Skip to content

Write MATLAB field classes EEGLAB expects on save - #320

Merged
arnodelorme merged 2 commits into
developfrom
fix/saveset-matlab-field-types
Sep 11, 2026
Merged

arnodelorme merged 2 commits into
developfrom
fix/saveset-matlab-field-types

Conversation

@arnodelorme

Copy link
Copy Markdown
Collaborator

🤖 Write the MATLAB classes EEGLAB expects when saving a .set file.

Problem

Loading an EEGPrep-written .set in MATLAB (class_compare of every struct field against the original EEGLAB file, R2025b) showed:

  • epoch(k).event as int64, and epoch(k).eventlatency / eventtype / eventposition / eventurevent as plain double / char matrices where EEGLAB builds cell arrays.
  • event.position, event.urevent, urevent.position, urchanlocs.theta etc., reject.thresh*, icachansind as int64; reject.gcompreject as uint8. MATLAB integer arithmetic rounds (int64(3) * 1000 / 128 is 23), so any EEGLAB code doing math on these fields silently misbehaves.
  • roi as a 1x1 struct instead of [], splinefile / icasplinefile as [] instead of '', epochdescription / eventdescription as [] instead of {}.

Fix (pop_saveset.py)

  • _matlab_double: recursive cast of Python/NumPy integers to double across the exported struct (dicts, lists, object arrays, and the object-typed rows built by flatten_dict). Booleans stay boolean so savemat writes MATLAB logical for masks such as etc.clean_sample_mask.
  • _epoch_fields_to_matlab: follows the eeg_checkset.m rule. epoch.event is a double row vector; each event<field> is a cell array unless no epoch holds more than one event (maxlen <= 1), in which case it is the bare value.
  • chanlocs.urchan written as double instead of int32.
  • Empty roi -> [], spline file names -> '', empty description fields -> {}.

In-memory EEG dicts are unchanged; the caller's data is deep-copied before conversion as before.

Verification

  • New tests in tests/test_pop_saveset.py: raw scipy.io.loadmat of the saved epoched sample checks double/cell classes for epoch, event, chanlocs, urchanlocs, reject, and icachansind, plus an in-memory round trip; a second test reduces the sample to one event per epoch and checks the scalar (non-cell) form.
  • MATLAB R2025b + vendored EEGLAB: after this change the per-field class comparison reports no numeric or epoch class differences for eeglab_data_epochs_ica.set and eeglab_data.set; epoch(k).event matches the original for all 80 epochs, eventurevent == event(epoch.event).urevent, data bit-identical, eeg_checkset(EEG, 'eventconsistency') clean.
  • Remaining differences, left alone: data single vs double (EEGLAB also writes single), chaninfo.filecontent cell vs char matrix, eventdescription element class, nested reject.disprej [] vs {}.
EEGPREP_SKIP_MATLAB=1 QT_QPA_PLATFORM=offscreen uv run pytest tests -m "not slow and not matlab and not octave and not gui and not visual"
# 1877 passed, 69 skipped (two pre-existing env-only failures deselected: math_backend ordering flake, torch missing)
./pre-commit.py, ruff check, ruff format --check: clean

Follows #314.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped: your organization's credit balance is too low for Code Review to run.

An organization admin can purchase credits or upgrade the plan in Plans & Billing.

Once credits are available, reopen this pull request to trigger a review.

@claude

claude Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@arnodelorme

Copy link
Copy Markdown
Collaborator Author

🤖 ## Code review

  • Overall assessment: Looks good. On-disk classes verified in MATLAB R2025b with the vendored EEGLAB.
  • Highest-risk area: _matlab_double recursing over the whole export dict (checked: float data/icaact arrays pass through untouched, recursion only walks metadata).
  • Merge recommendation: Safe to merge after a trivial rebase. The only conflict with develop is adjacent bullets in docs/source/user_guide/concepts.rst.

Blocking

None.

Important

  • GitHub reports CONFLICTING, but the only conflicted file is docs/source/user_guide/concepts.rst (this PR's bullet vs. the Keep dataset numbers stable when deleting datasets #322 bullets added right below it). Keep both.
  • Merge order with Use 0-based urevent consistently in memory #318: both PRs append tests at the same place in tests/test_pop_saveset.py, so whichever lands second gets one add/add conflict there. Trivial, keep both.
  • The red "Ruff and ty" / "Build Documentation" checks are stale (docs example formatting and __file__ errors fixed on develop in 7a779dd); the test matrix was skipped because of them. Rebasing fixes CI.

Nits

None.

Test gaps

None. The raw scipy.io.loadmat assertions pin the classes, and the one-event-per-epoch test covers the scalar branch of _epoch_fields_to_matlab.

EEGLAB parity notes

Independent check: saved eeglab_data_epochs_ica.set with this branch's pop_saveset, loaded it in MATLAB R2025b with src/eegprep/eeglab pop_loadset, and compared against the original file:

data equal (single):                       yes
event.position/urevent/epoch/latency:      double   (orig double)
epoch.event:                               double [1 3], equal to original for all 80 epochs
epoch.eventlatency/eventtype/eventposition: cell [1 3] (orig cell)
epoch.eventurevent:                        cell [1 3] (orig file has legacy char; cell is what eeg_checkset.m builds)
eventurevent == [event(epoch.event).urevent]: yes
chanlocs.urchan, urchanlocs.theta, icachansind, reject.gcompreject/threshentropy: double
roi: [] double 0x0   epochdescription: {} 0x0   splinefile/icasplinefile: ''
eeg_checkset(EEG, 'eventconsistency'): OK
one-event-per-epoch file: eventtype char, eventlatency/event double scalars, eventconsistency OK
  • flatten_dict's non-object path already casts numeric fields to float64, so _matlab_double on the object-tuple path plus the top-level pass closes the remaining int64 routes. Consistent.

Python verification on develop (b377c3f) + this branch (e516e0d):

pytest tests/test_pop_saveset.py tests/test_pop_loadset.py tests/test_eeg_checkset.py tests/test_pop_epoch.py tests/test_pop_editset.py tests/test_sample_data_pop_functions.py
169 passed, 10 skipped (MATLAB)
pytest tests/test_pop_loadset_h5.py -k "not Parity"
18 passed

…-field-types

# Conflicts:
#	docs/source/user_guide/concepts.rst
@arnodelorme
arnodelorme merged commit d5d57a0 into develop Sep 11, 2026
3 checks passed
@arnodelorme
arnodelorme deleted the fix/saveset-matlab-field-types branch September 11, 2026 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant